Fix some UART-related issues #4586
Conversation
|
/hil full |
|
Triggered full HIL run for #4586. Run: https://github.com/esp-rs/esp-hal/actions/runs/19830655480 Status update: ❌ HIL (full) run failed (conclusion: failure). |
bugadani
force-pushed
the
uart-changes
branch
from
4ad6928 to
bed8a12
Compare
bugadani
force-pushed
the
uart-changes
branch
4 times, most recently
from
ef47d58 to
6c02337
Compare
|
/hil full |
|
Triggered full HIL run for #4586. Run: https://github.com/esp-rs/esp-hal/actions/runs/19865042209 Status update: ❌ HIL (full) run failed (conclusion: failure). |
bugadani
force-pushed
the
uart-changes
branch
from
fd07a6e to
1f46865
Compare
|
I just tested the current commit of your branch |
|
Thank you, I'll spend time to try and better reproduce this issue. In the mean time, can you try changing the |
bugadani
force-pushed
the
uart-changes
branch
from
1f46865 to
175cfb6
Compare
|
I think there are enough changes here to move forward. While this doesn't fix #4523, the PR fixes some minor perf issues and doesn't seem to make things worse with them. |
|
/hil full |
|
Triggered full HIL run for #4586. Run: https://github.com/esp-rs/esp-hal/actions/runs/19888034284 Status update: HIL (full) run is still in progress or status unknown._ |
There was a problem hiding this comment.
Pull request overview
This PR fixes several UART-related issues including unclear documentation and potential race conditions in async read operations. The changes primarily focus on improving the reliability of read_exact_async and clarifying blocking behavior in documentation.
- Corrects misleading documentation that stated
readmethods would not block - Refactors
wait_for_buffered_datato fix issue #4523 by changing from awhileloop to conditional waiting - Adds buffer draining in
read_exact_asyncto prevent unexpectedFifoOverflowederrors
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| esp-hal/src/uart/mod.rs | Refactors async UART receive logic to prevent race conditions, updates blocking behavior documentation for read/write_ready/read_ready methods, and adds defensive event clearing in read_buffered |
| esp-hal/CHANGELOG.md | Documents the documentation corrections and bug fix for read_exact_async |
Comments suppressed due to low confidence (1)
esp-hal/src/uart/mod.rs:1192
- [nitpick] Inconsistent method call pattern detected. Lines 1182 and 1192 call
self.uart.info().read_buffered(buf)directly, while line 1159 (inread_async) callsself.read_buffered(buf).
For consistency and better encapsulation, consider using self.read_buffered(buf) instead, which internally calls self.uart.info().read_buffered(buf) as seen in line 1351.
let read = self.uart.info().read_buffered(buf)?;
buf = &mut buf[read..];
while !buf.is_empty() {
// No point in listening for timeouts, as we're waiting for an exact amount of
// data. On ESP32 and S2, the timeout interrupt can't be cleared unless the FIFO
// is empty, so listening could cause an infinite loop here.
self.wait_for_buffered_data(buf.len(), buf.len(), false)
.await?;
let read = self.uart.info().read_buffered(buf)?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MabezDev
left a comment
MabezDev
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Should we hold off on the backport PR until the OG issue is resolved?
|
I think this is valuable enough on its own. |
MabezDev
added
the
esp-hal-backport
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin esp-hal-1.0.x
git worktree add -d .worktree/backport-4586-to-esp-hal-1.0.x origin/esp-hal-1.0.x
cd .worktree/backport-4586-to-esp-hal-1.0.x
git switch --create backport-4586-to-esp-hal-1.0.x
git cherry-pick -x d3d2a77759e5c5e0623055f093c5f4b2515414f4 |
3 participants
This PR makes three changes to the UART driver:
It stabilizesWill be proposed separately.read_readyand expands on its documentation, to make it possible to truly read without blocking.wait_for_buffered_data:read_exact_async@jimy-byerley could you please see if this PR addresses your issue? It's not easy to reproduce on the MCU itself, and I'm not sure if we can make a reliable test case for it.